Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update k8s Dependencies #4199

Merged
merged 17 commits into from
Oct 20, 2023
Merged

Update k8s Dependencies #4199

merged 17 commits into from
Oct 20, 2023

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Oct 10, 2023 •
edited
Loading

Describe your changes

  • Update K8s dependencies and APIs that changed
  • Remove sagemaker plugin since:
    • It pins old versions of K8s deps
    • I'm told is broken
    • Uses a deprecated AWS Operator
    • Should be replaced with a Flyte Agent.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

EngHabu added 2 commits October 9, 2023 20:55
@EngHabu
Update k8s deps for flytepropeller
9a33305 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
wip
638a8d1 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@codecov
Copy link

codecov bot commented Oct 11, 2023 •
edited
Loading

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (e1c05db) 59.31% compared to head (1a298dd) 59.89%.

❗ Current head 1a298dd differs from pull request most recent head 738be36. Consider uploading reports for the commit 738be36 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4199      +/-   ##
==========================================
+ Coverage   59.31%   59.89%   +0.58%     
==========================================
  Files         623      562      -61     
  Lines       53394    40224   -13170     
==========================================
- Hits        31669    24092    -7577     
+ Misses      19204    13789    -5415     
+ Partials     2521     2343     -178
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
flytepropeller/pkg/controller/garbage_collector.go 74.31% <100.00%> (+1.51%) ⬆️
...er/pkg/controller/nodes/task/backoff/controller.go 0.00% <ø> (ø)
...eller/pkg/controller/nodes/task/backoff/handler.go 70.27% <ø> (-6.80%) ⬇️
flytepropeller/pkg/compiler/validators/utils.go 65.75% <87.50%> (+1.33%) ⬆️
flytepropeller/pkg/controller/executors/kube.go 66.66% <0.00%> (+30.55%) ⬆️
...ler/pkg/controller/nodes/task/k8s/event_watcher.go 75.00% <25.00%> (+1.15%) ⬆️
...er/pkg/controller/nodes/task/k8s/plugin_manager.go 53.51% <20.00%> (+1.21%) ⬆️
flytepropeller/pkg/controller/controller.go 11.37% <0.00%> (-0.32%) ⬇️

... and 562 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EngHabu EngHabu changed the title Update k8s dips Update k8s Dependencies Oct 18, 2023
EngHabu added 11 commits October 18, 2023 16:59
@EngHabu
cleanup
4436f00 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
go mod tidy
f134992 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Move some pipelines to union-k8s-runner
4ab3960 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
wip
63d0bd0 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
run on ubuntu because we run out of disk otherwise
0d28254 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
nvm union machines
c893bca 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
lint for plugins
5f482ba 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
lint
64412ae 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
fix unit tests
02bc723 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Merge branch 'master' of github.com:flyteorg/flyte into update-k8s-dips
6e884a6
@EngHabu
revert change in test... probably osx diff
876abc3 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu marked this pull request as ready for review October 19, 2023 23:15
@EngHabu
Remove sagemaker proto & docs
423b0e5 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
pingsutw
pingsutw reviewed Oct 20, 2023
View reviewed changes
.github/workflows/stale.yml Outdated Show resolved Hide resolved
.github/workflows/codespell.yml Outdated
@@ -17,7 +17,7 @@ permissions:
jobs:
codespell:
name: Check for spelling errors
runs-on: ubuntu-latest
runs-on: union-k8s-runner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: will this GA workflows run on our cluster? If so, should we only run unit and integration test on our cluster? That said, we can use github action to run some other small workflow, such as check for spelling errors, lint, make generate, and push image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rethinking this, Union cluster need to have machines with bigger attached disks, the default ones run out of capacity really quickly and can't even run the unit tests... I'll revert this change and think about this separately.

@EngHabu
Revert running on union CI cluster
738be36 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu requested a review from pingsutw October 20, 2023 15:58
@EngHabu EngHabu merged commit 8749fce into master Oct 20, 2023
43 of 44 checks passed
@EngHabu EngHabu deleted the update-k8s-dips branch October 20, 2023 17:42
@eapolinario
Copy link
Contributor

I'm reverting this temporarily just to get 1.10 out. The reason being that I don't want to ship these changes without (a lot) more testing.

eapolinario added a commit that referenced this pull request Oct 20, 2023
@eapolinario
Revert "Update k8s Dependencies (#4199)"
e187ad3 
This reverts commit 8749fce.
@eapolinario eapolinario mentioned this pull request Oct 20, 2023
Merged
eapolinario added a commit that referenced this pull request Oct 20, 2023
@eapolinario
Revert "Update k8s Dependencies (#4199)" (#4272)
6f02b4c 
This reverts commit 8749fce.
eapolinario added a commit that referenced this pull request Oct 20, 2023
@eapolinario
Revert "Revert "Update k8s Dependencies (#4199)" (#4272)"
9fbb52e 
This reverts commit 6f02b4c.
EngHabu pushed a commit that referenced this pull request Oct 21, 2023
@eapolinario
Revert "Revert "Update k8s Dependencies (#4199)" (#4272)" (#4273)
5017e6c 
This reverts commit 6f02b4c.
Tom-Newton
Tom-Newton reviewed Nov 27, 2023
View reviewed changes
flytepropeller/pkg/controller/garbage_collector.go
@@ -162,7 +162,7 @@ func (g *GarbageCollector) StartGC(ctx context.Context) error {
logger.Warningf(ctx, "Garbage collector is disabled, as ttl [%d] is <=0", g.ttlHours)
return nil
}
ticker := g.clk.NewTicker(g.interval)
ticker := g.clk.NewTimer(g.interval)
Copy link
Contributor

@Tom-Newton Tom-Newton Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you explain why was this changed from a ticker to a timer? I'm a total golang noob but I think this means that flytepropeller will only garbage collect once each time it restarts.

I think this is causing us some problems https://flyte-org.slack.com/archives/CP2HDHKE1/p1701118219768919?thread_ts=1700590318.336129&cid=CP2HDHKE1

@hamersaw hamersaw mentioned this pull request Dec 4, 2023
Open
2 tasks
This was referenced Jan 12, 2024
Closed
Merged
@honnix honnix mentioned this pull request Jan 19, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tom-Newton Tom-Newton Tom-Newton left review comments

@pingsutw pingsutw pingsutw approved these changes

@eapolinario eapolinario Awaiting requested review from eapolinario

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EngHabu @eapolinario @Tom-Newton @pingsutw